Skip to content

feat(mail): send idempotency + delivery worker (HT-16) - #21

Merged
zaridan merged 3 commits into
mainfrom
feat/ht-16-send-idempotency
Jul 11, 2026
Merged

feat(mail): send idempotency + delivery worker (HT-16)#21
zaridan merged 3 commits into
mainfrom
feat/ht-16-send-idempotency

Conversation

@zaridan

@zaridan zaridan commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Combines a caller-supplied idempotencyKey with a creation/delivery split: a retry finds the existing outbound row via an atomic get-or-insert and reuses its message_id and stored send_envelope verbatim — never re-mints, never recomputes.
  • Migration 003 (threads.idempotency_key / send_envelope / claimed_until, all outbound-only) plus a partial unique index on (conversation_id, idempotency_key) are the schema-level mechanism; see the migration's doc comment for the NULL-semantics and backfill reasoning.
  • ConversationStore.appendThread now resolves a keyed call as INSERT ... ON CONFLICT ... DO NOTHING RETURNING *, falling back to a SELECT of the existing row on conflict — inside the SAME transaction that already takes the FOR UPDATE lock on the conversation row, so a concurrent retry with the same key is serialized rather than racing.
  • claimThreadForDelivery / releaseThreadLease are a lease pair (a plain, row-locked UPDATE ... WHERE claimed_until IS NULL OR claimed_until < now()) shared by sendReply's keyed-retry path and the new runDeliveryWorker sweep, so at most one attempt is ever sending a given row at a time.
  • src/mail/delivery-worker.ts is a plain invocable sweep function (runDeliveryWorker), not built on QueueProvider/SchedulerProvider — no such adapter exists yet, so wiring a real schedule (Vercel Cron, or a future SchedulerProvider adapter) is deferred to a one-line call to this function later.
  • POST /api/v1/conversations/{id}/replies now requires an Idempotency-Key header (400 if missing/empty) — a deliberate breaking change, since this endpoint is dogfood-only. A replay of the same key on the same conversation returns the original outcome without re-diffing the body or re-invoking the sender; a lease held by a concurrent attempt maps to 409 retry_in_progress.
  • specs/mail/sending.md §3a and specs/api/agent-inbox-v1.md §4a updated to close their HT-16 forward references.

Mail-semantics equivalence evidence (CHARTER.md invariant #5)

The no-idempotencyKey send path is byte-identical to pre-HT-16 behavior — no fixtures needed here since none existed before, but the strongest evidence available: all 7 pre-existing tests in src/mail/send.test.ts pass completely UNEDITED against the new sendReply, along with every pre-existing test in src/store/conversations.test.ts (only one assertion's shape was widened, from an exact toEqual to a toMatchObject, to accommodate the new created/thread fields on AppendResult — no behavioral change). Threading (thread.ts), parsing (parse.ts), Message-ID minting (reply-token.ts), and all fixture-based tests are untouched and still pass. Full suite: 320/320 tests passing.

Verification (all exit 0)

  • npx tsc --noEmit -p tsconfig.json — clean, no errors.
  • npx biome check . — clean, no errors (after auto-formatting fixes).
  • npx vitest run320 passed (320), 17 test files.
  • npx vitest run --coverage — 320/320 passing; overall 94.92% stmts / 90.29% branch / 96.36% funcs / 95.61% lines (no threshold configured in vitest.config, so this is informational).

Notes / flagged items

  • Breaking change: Idempotency-Key is now required on POST .../replies. Dogfood-only endpoint, no external consumer today.
  • Deferred: the delivery worker is a plain sweep function; queue/scheduler adapter wiring (a SchedulerProvider calling it on a real cron/interval) is intentionally out of scope for this increment, per the approved design.
  • One necessary extension beyond the literal design bullets: ConversationStore gained listDeliverableThreads (not explicitly named in the design's store bullet list) — required for the worker's eligibility sweep, and send_envelope is now persisted on every outbound send (keyed or not), not only keyed ones — this is what lets the worker retry a pre-existing no-key pending/failed row too, and is implied by the design's own parenthetical ("the envelope now gets persisted via sendEnvelope on insert").

Adversarial review (pre-human-review)

  • A (MAJOR, spec): sending.md implied at-most-once delivery; the implementation is at-least-once. §3a now states this explicitly, names the concrete residual (provider accepts → mark-sent write fails → row stays pending with a live envelope → once stale/lease-free, a worker or keyed replay re-sends an already-delivered message — the engine cannot distinguish "crashed before send" from "sent but unmarked"), and §4 elevates provider Message-ID dedup from an aside to a stated precondition for true at-most-once.
  • B (MAJOR, code+spec): DEFAULT_LEASE_MS raised from 30s to 120s; its doc comment (and sending.md §3a/§4) now states the invariant explicitly — the lease MUST strictly exceed the worst-case EmailSender.send() duration, or a re-claimed retry can race the original call into a genuine concurrent double-send. Checked the Gmail adapter (src/providers/adapters/gmail/sender.ts) per the instruction not to modify it here: it already bounds its HTTP call with an explicit AbortSignal.timeout, default 30 000 ms (configurable via timeoutMs), comfortably under the new 120s lease — no code change needed, but see the follow-up note below.
  • C (MINOR, docs): The attemptDeliveryOfClaimedThread comment for "sent but mark-sent failed" implied the row staying claimed meaningfully delays a resend. Corrected: the lease is a fraction of the delivery worker's 5-minute staleAfterMs, and the no-key path never claims at all — the real backstop is provider Message-ID dedup (per finding A), not the claimed state.
  • D (MINOR, test honesty): Added a caveat comment (matching migrate.ts's advisory-lock caveat) to the three concurrency tests that run against single-connection PGlite — conversations.test.ts's Promise.all same-key test, send.test.ts's in-flight-lease test, and delivery-worker.test.ts's cross-path race test — noting they prove sequential claim-while-held logic, not true multi-connection atomicity; that coverage waits for a multi-connection backend.
  • E (MINOR, code): Idempotency-Key is now trimmed before use and rejected with 400 validation_failed if empty after trimming or over 255 characters; the trimmed value is what's stored and passed to sendReply. Added tests: a whitespace-padded key (NBSP, since the Headers implementation already strips plain HTTP OWS) replays the same send as its trimmed twin (one send only), and a >255-char key is rejected. agent-inbox-v1.md §4a updated.
  • F (NOTE, spec): agent-inbox-v1.md §4a now states that a keyed replay after the conversation has been deleted returns 404, not the original 201 — replay-of-original-outcome does not survive a conversation delete (no mail-safety impact; the original send already happened).
  • G: already documented.

Follow-up (not in this PR): the Gmail adapter's 30s default timeout is safely under the new 120s lease today, but nothing ties the two together — a future change to either constant could silently violate the invariant B documents. Worth a lint/test assertion or a shared-constant follow-up ticket.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added idempotent reply sending with a required, conversation-scoped Idempotency-Key (trimmed; max 255 chars) including safe replays for success and retry recovery.
    • Introduced lease-based delivery retry sweep to re-attempt stale failed/pending sends while preventing duplicate in-flight deliveries.
  • Bug Fixes
    • Improved request validation and error mapping (e.g., invalid/blank keys return 400 validation_failed; retry_in_progress now returns HTTP 409 retry_in_progress).
    • Clarified missing/deleted conversation handling and 502 send_failed behavior on provider failures.
  • Documentation
    • Updated Agent Inbox and mail sending specs for the refined status/error, idempotency, replay, and lease semantics.
  • Tests
    • Expanded API, store, migration, and delivery-worker coverage for the new idempotency/lease rules.

Adds a caller-supplied idempotency key to the reply send path plus a plain
delivery-worker sweep function, closing the "idempotency is NOT yet handled
here (HT-16)" TODO in src/mail/send.ts.

- Migration 003: threads.idempotency_key/send_envelope/claimed_until,
  outbound-only, with a partial unique index for atomic get-or-insert.
- Store: appendThread's INSERT...ON CONFLICT...DO NOTHING get-or-insert (same
  transaction as its existing FOR UPDATE lock), plus claimThreadForDelivery/
  releaseThreadLease/listDeliverableThreads for the delivery lease.
- send.ts: sendReply branches on delivery_status for a keyed retry (replay
  success, claim-then-resend, or retry-in-progress); attemptDeliveryOfClaimedThread
  is the shared helper the new delivery-worker.ts sweep also calls. The no-key
  path is untouched — all 7 pre-existing send.test.ts tests pass unedited.
- API: POST .../replies now requires an Idempotency-Key header (400 if
  missing); retry-in-progress maps to 409 retry_in_progress. Breaking change,
  dogfood-only endpoint.
- specs/mail/sending.md and specs/api/agent-inbox-v1.md updated to close
  their HT-16 forward references.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 852a3e69-82e8-48e5-8a4a-62802b7649a8

📥 Commits

Reviewing files that changed from the base of the PR and between bd71ed8 and ccc9be2.

📒 Files selected for processing (6)
  • specs/mail/sending.md
  • src/mail/delivery-worker.test.ts
  • src/mail/send.test.ts
  • src/mail/send.ts
  • src/store/conversations.test.ts
  • src/store/conversations.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/mail/delivery-worker.test.ts
  • src/store/conversations.test.ts
  • specs/mail/sending.md
  • src/mail/send.ts
  • src/store/conversations.ts

📝 Walkthrough

Walkthrough

The change adds conversation-scoped reply idempotency, persisted outbound envelopes, delivery leases, retry sweeping, migration support, API validation, replay handling, and related specifications and tests.

Changes

Idempotent outbound delivery

Layer / File(s) Summary
Contracts and persistence schema
specs/api/agent-inbox-v1.md, specs/mail/sending.md, src/db/migrate.ts, src/store/conversations.ts
Defines idempotency keys, immutable send envelopes, delivery leases, replay outcomes, and outbound-only database constraints.
Atomic thread insertion and leasing
src/store/conversations.ts, src/store/conversations.test.ts, src/db/migrate.test.ts
Implements atomic get-or-insert behavior, preserves replayed rows, and adds lease claim, release, and eligible-thread selection logic.
Keyed reply sending and replay
src/mail/send.ts, src/mail/send.test.ts
Reuses stored identifiers and envelopes for retries, prevents concurrent duplicate sends, and records delivery outcomes.
Delivery retry sweep
src/mail/delivery-worker.ts, src/mail/delivery-worker.test.ts
Adds a configurable worker that claims stale or failed threads, retries delivery, and reports sent, failed, and skipped counts.
Reply endpoint enforcement
src/api/conversations.ts, src/api/index.test.ts
Requires a trimmed, bounded Idempotency-Key, forwards it to sending, and maps lease contention to 409 retry_in_progress.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant handleReply
  participant sendReply
  participant ConversationStore
  participant EmailSender
  Client->>handleReply: POST reply with Idempotency-Key
  handleReply->>sendReply: send keyed reply
  sendReply->>ConversationStore: get-or-insert thread and claim lease
  ConversationStore-->>sendReply: stored thread or retry-in-progress
  sendReply->>EmailSender: send stored envelope
  EmailSender-->>sendReply: delivery result
  sendReply->>ConversationStore: release lease and persist status
  sendReply-->>handleReply: result
  handleReply-->>Client: HTTP response
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 56.25% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: mail send idempotency and the delivery worker for HT-16.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ht-16-send-idempotency

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
src/db/migrate.ts (1)

105-174: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

claimed_until isn't actually constrained to outbound-only, despite the doc comment's claim.

The doc comment says all three new columns are outbound-only, but only idempotency_key and send_envelope get a CHECK constraint enforcing that; claimed_until has none. Today this is only enforced via the app-level direction = 'outbound' scoping in claimThreadForDelivery/releaseThreadLease — a schema-level gap relative to the other two columns and the stated invariant.

🛡️ Proposed fix
 ALTER TABLE threads ADD CONSTRAINT threads_send_envelope_outbound_only CHECK (
   (direction = 'outbound') OR (send_envelope IS NULL)
 );
+ALTER TABLE threads ADD CONSTRAINT threads_claimed_until_outbound_only CHECK (
+  (direction = 'outbound') OR (claimed_until IS NULL)
+);
 CREATE UNIQUE INDEX threads_conversation_idempotency_key_idx ON threads (conversation_id, idempotency_key) WHERE idempotency_key IS NOT NULL;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/db/migrate.ts` around lines 105 - 174, Update the
MIGRATION_003_SEND_IDEMPOTENCY schema to add an outbound-only CHECK constraint
for claimed_until, matching the existing threads_idempotency_key_outbound_only
and threads_send_envelope_outbound_only constraints: inbound rows must have
claimed_until NULL, while outbound rows may contain either value.
src/store/conversations.test.ts (1)

544-574: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

This "concurrent" test doesn't exercise real DB-level concurrency.

PGlite runs in Postgres single-user mode with one exclusive connection, so Promise.all-ing two appendThread calls against the same Db instance still serializes them at the wire-protocol level — this test proves the second sequential call correctly finds the first's committed row, not that the FOR UPDATE lock + ON CONFLICT combination is race-safe under genuinely overlapping transactions (e.g. two processes, or two real connections). The comment at line 570 ("Exactly one of the two calls actually created the row") reads as if this validates the race, but PGlite's single-connection model can't produce that race in the first place.

Consider either softening the test's framing (it validates sequential get-or-insert correctness, not concurrency safety) or, if true concurrency coverage is wanted, exercising it against a real multi-connection Postgres in CI.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/store/conversations.test.ts` around lines 544 - 574, Reframe the test
named “concurrent appendThread calls with the SAME key resolve to exactly one
created row” as sequential or single-connection get-or-insert coverage, since
Promise.all on the shared PGlite store does not create real database
concurrency. Update its description and the “Exactly one...” assertion comment
to avoid claiming race-safety; only introduce multi-connection Postgres coverage
if genuine concurrency testing is required.
src/mail/delivery-worker.ts (1)

100-116: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

Sequential per-candidate processing caps sweep throughput.

Each candidate is claimed and sent fully serially. Since each candidate has its own independent lease, these could be processed with bounded concurrency (e.g. a small worker pool) to reduce total sweep latency when batchSize candidates are all genuinely eligible. Given the documented intent of keeping this a simple, low-risk sweep function, this is a nice-to-have rather than a blocker.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mail/delivery-worker.ts` around lines 100 - 116, Update the
candidate-processing loop in the delivery sweep to process independent claims
and delivery attempts with bounded concurrency rather than fully serial
execution. Preserve the existing claim, sent, failed, and skipped accounting,
and keep the concurrency limit small and explicit so the sweep remains simple
and low risk.
src/mail/send.ts (1)

251-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

thread.messageId as string bypasses the null-check pattern used elsewhere.

The invariant (deliveryStatus 'sent'messageId non-null) is presumably schema-enforced, but attemptDeliveryOfClaimedThread (below) uses an explicit runtime check instead of a type assertion for the same invariant. For consistency and to fail loudly instead of silently returning a bogus value if the invariant is ever violated, prefer the same explicit check here.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mail/send.ts` around lines 251 - 260, Replace the thread.messageId as
string assertion in the sent-status replay branch of
attemptDeliveryOfClaimedThread with an explicit runtime null check matching the
check used in the delivery attempt path. Fail loudly when messageId is missing,
while preserving the existing successful replay response when it is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/api/conversations.ts`:
- Around line 240-244: Normalize the Idempotency-Key once in the conversation
request flow by trimming the header value after retrieval, validate the
normalized value for emptiness, and reuse that normalized key in the sendReply
call. Update both the validation block and sendReply argument so
whitespace-padded representations map to the same downstream key.

In `@src/mail/send.ts`:
- Around line 340-419: Prevent duplicate delivery by ensuring
attemptDeliveryOfClaimedThread does not call sender.send for threads whose
deliveryStatus is already sent. Prefer adding delivery_status IN
('pending','failed') to claimThreadForDelivery, or add an explicit sent-status
guard before the send while preserving the existing result contract.

---

Nitpick comments:
In `@src/db/migrate.ts`:
- Around line 105-174: Update the MIGRATION_003_SEND_IDEMPOTENCY schema to add
an outbound-only CHECK constraint for claimed_until, matching the existing
threads_idempotency_key_outbound_only and threads_send_envelope_outbound_only
constraints: inbound rows must have claimed_until NULL, while outbound rows may
contain either value.

In `@src/mail/delivery-worker.ts`:
- Around line 100-116: Update the candidate-processing loop in the delivery
sweep to process independent claims and delivery attempts with bounded
concurrency rather than fully serial execution. Preserve the existing claim,
sent, failed, and skipped accounting, and keep the concurrency limit small and
explicit so the sweep remains simple and low risk.

In `@src/mail/send.ts`:
- Around line 251-260: Replace the thread.messageId as string assertion in the
sent-status replay branch of attemptDeliveryOfClaimedThread with an explicit
runtime null check matching the check used in the delivery attempt path. Fail
loudly when messageId is missing, while preserving the existing successful
replay response when it is present.

In `@src/store/conversations.test.ts`:
- Around line 544-574: Reframe the test named “concurrent appendThread calls
with the SAME key resolve to exactly one created row” as sequential or
single-connection get-or-insert coverage, since Promise.all on the shared PGlite
store does not create real database concurrency. Update its description and the
“Exactly one...” assertion comment to avoid claiming race-safety; only introduce
multi-connection Postgres coverage if genuine concurrency testing is required.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ba1a5f5-7de3-4051-b9d1-d58f3dceeaa8

📥 Commits

Reviewing files that changed from the base of the PR and between 5608890 and 9f812bd.

📒 Files selected for processing (12)
  • specs/api/agent-inbox-v1.md
  • specs/mail/sending.md
  • src/api/conversations.ts
  • src/api/index.test.ts
  • src/db/migrate.test.ts
  • src/db/migrate.ts
  • src/mail/delivery-worker.test.ts
  • src/mail/delivery-worker.ts
  • src/mail/send.test.ts
  • src/mail/send.ts
  • src/store/conversations.test.ts
  • src/store/conversations.ts

Comment thread src/api/conversations.ts Outdated
Comment thread src/mail/send.ts
…ease/latency coupling, key validation

Corrects sending.md's at-most-once implication to at-least-once (with the
concrete residual: accept-then-mark-fails leaves a stale pending row that
gets re-sent), raises DEFAULT_LEASE_MS to 120s with the lease/send-duration
invariant spelled out, corrects a misleading "left claimed delays resend"
comment, adds real-race caveats to the single-connection PGlite concurrency
tests, and validates+trims the Idempotency-Key header (400 on empty-after-
trim or >255 chars).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
specs/mail/sending.md (2)

89-100: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Do not promise byte-identical wire output from a structured snapshot.

send_envelope stores structured fields, and retries reconstruct OutboundEmail; raw MIME bytes and provider serialization are not persisted. Narrow this to field/semantic identity, or persist canonical MIME and prove wire equivalence with fixtures.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@specs/mail/sending.md` around lines 89 - 100, Narrow the retry guarantee in
the send_envelope specification from byte-identical wire output to preserving
the same stored envelope fields and email semantics. Update the affected
statements to acknowledge that retries reconstruct OutboundEmail and provider
serialization may differ; do not claim wire equivalence unless canonical MIME is
persisted and validated.

102-118: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Fence lease release to the claimant.

claimThreadForDelivery and releaseThreadLease are keyed only by threadId (src/store/conversations.ts:592-643). If a lease expires before the post-send release, another attempt can claim/send, then the original attempt can clear the new lease or overwrite its status. A send-duration bound does not cover process pauses or delayed database writes. Return a claim token/version and require it for release and status updates.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@specs/mail/sending.md` around lines 102 - 118, Update claimThreadForDelivery
and releaseThreadLease to use a claimant token or lease version returned by the
claim and required by release and sent/failed status updates. Ensure each
mutation only succeeds when the stored token/version still matches the claimant,
preventing an expired lease holder from clearing or overwriting a newer claim;
propagate this token through both retry and delivery-worker send paths.
src/mail/send.ts (2)

260-310: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Keep the sent-state check atomic with the claim.

The pre-claim deliveryStatus === 'sent' shortcut does not close the listing/claim race. claimThreadForDelivery currently checks only lease availability, so a worker can claim a row that another retry marked sent and resend it. Filter delivery_status IN ('pending', 'failed') in the atomic claim or reject sent rows before sender.send(). This remains the previously reported issue.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mail/send.ts` around lines 260 - 310, Make the sent-state guard atomic
with delivery claiming: update claimThreadForDelivery so it only claims rows
whose delivery_status is pending or failed, preventing a concurrent worker from
claiming a row marked sent. Preserve the existing retry-in-progress behavior and
ensure attemptDeliveryOfClaimedThread never calls sender.send for a sent row.

430-449: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Expose the unmarked-success state to the worker.

When releaseThreadLease(..., 'sent') fails, this function still returns ok: true, while runDeliveryWorker counts result.ok as sent even though the row remains pending. Return the persisted status (or a distinct sent-unmarked result), or adjust the worker report contract so monitoring does not claim reconciliation succeeded.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mail/send.ts` around lines 430 - 449, The attemptDeliveryOfClaimedThread
success path must expose when delivery succeeded but releaseThreadLease(...,
'sent') failed instead of returning an undifferentiated ok: true. Update this
function and the runDeliveryWorker result/report handling to preserve a distinct
sent-unmarked or persisted-status outcome, ensuring monitoring does not count
the still-pending row as reconciled sent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@specs/mail/sending.md`:
- Around line 171-182: Resolve the inconsistency between the “Precondition”
heading and the provider deduplication “SHOULD” in this section: either make
Message-ID deduplication a mandatory deployment requirement for at-most-once
delivery, or rename the heading to describe it as a recommendation while
preserving the documented at-least-once behavior when unsupported.

---

Outside diff comments:
In `@specs/mail/sending.md`:
- Around line 89-100: Narrow the retry guarantee in the send_envelope
specification from byte-identical wire output to preserving the same stored
envelope fields and email semantics. Update the affected statements to
acknowledge that retries reconstruct OutboundEmail and provider serialization
may differ; do not claim wire equivalence unless canonical MIME is persisted and
validated.
- Around line 102-118: Update claimThreadForDelivery and releaseThreadLease to
use a claimant token or lease version returned by the claim and required by
release and sent/failed status updates. Ensure each mutation only succeeds when
the stored token/version still matches the claimant, preventing an expired lease
holder from clearing or overwriting a newer claim; propagate this token through
both retry and delivery-worker send paths.

In `@src/mail/send.ts`:
- Around line 260-310: Make the sent-state guard atomic with delivery claiming:
update claimThreadForDelivery so it only claims rows whose delivery_status is
pending or failed, preventing a concurrent worker from claiming a row marked
sent. Preserve the existing retry-in-progress behavior and ensure
attemptDeliveryOfClaimedThread never calls sender.send for a sent row.
- Around line 430-449: The attemptDeliveryOfClaimedThread success path must
expose when delivery succeeded but releaseThreadLease(..., 'sent') failed
instead of returning an undifferentiated ok: true. Update this function and the
runDeliveryWorker result/report handling to preserve a distinct sent-unmarked or
persisted-status outcome, ensuring monitoring does not count the still-pending
row as reconciled sent.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8630f422-e1b3-4042-a5ba-9bd9e100ec56

📥 Commits

Reviewing files that changed from the base of the PR and between 9f812bd and bd71ed8.

📒 Files selected for processing (8)
  • specs/api/agent-inbox-v1.md
  • specs/mail/sending.md
  • src/api/conversations.ts
  • src/api/index.test.ts
  • src/mail/delivery-worker.test.ts
  • src/mail/send.test.ts
  • src/mail/send.ts
  • src/store/conversations.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/api/conversations.ts
  • src/mail/delivery-worker.test.ts
  • src/store/conversations.test.ts
  • specs/api/agent-inbox-v1.md
  • src/mail/send.test.ts

Comment thread specs/mail/sending.md Outdated
…im double-send (CodeRabbit review)

claimThreadForDelivery's WHERE clause only checked the lease
(claimed_until), not delivery_status. releaseThreadLease clears
claimed_until in the same write that records the outcome, so a row that
reached 'sent' had a free lease and could be reclaimed and re-sent by a
concurrent keyed sendReply retry or the delivery worker. Add `AND
delivery_status IN ('pending', 'failed')` to the claim so a 'sent' row
can never be reclaimed, and teach sendReply's keyed path to re-read the
thread on a failed claim so a row found already 'sent' resolves to the
same success-replay result instead of a misleading 'retry-in-progress'.

Also rewords specs/mail/sending.md §4 to stop calling provider
Message-ID dedup a "precondition" — it's a SHOULD/recommendation; the
engine's at-least-once guarantee holds without it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zaridan

zaridan commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Disposition of the three CodeRabbit findings on this PR:

  1. Trim inconsistency — already fixed in bd71ed8, before this comment surfaced. The review comment was anchored to the first commit (9f812bd), which predates that fix.

  2. Sent-row reclaim (Major) — confirmed real, fixed here. claimThreadForDelivery's WHERE clause checked only the lease (claimed_until), not delivery_status. Since releaseThreadLease clears claimed_until in the same write that records the outcome, a row that reached 'sent' had a free lease and could be reclaimed — letting a stale worker listing or a keyed sendReply replay re-send an already-delivered message. Fixed in ccc9be2:

    • src/store/conversations.ts: added AND delivery_status IN ('pending', 'failed') to the claim's WHERE, so a 'sent' row can never be reclaimed — the status re-check rides the same row lock that already serializes concurrent claims.
    • src/mail/send.ts: on a failed claim, the keyed path now re-reads the thread; if it's 'sent', returns the same success-replay result as the early 'sent' short-circuit (sender not called) instead of a misleading retry-in-progress. Only a row still genuinely pending/failed (lease truly held) returns retry-in-progress.
    • Regression tests added at all three layers: store (conversations.test.ts — a 'sent' row with a free lease is unclaimable; 'pending'/'failed' rows remain claimable), send (send.test.ts — simulated TOCTOU between the get-or-insert snapshot and the claim call resolves to success-replay, sender never called), and worker (delivery-worker.test.ts — a row that turns 'sent' between listing and claim is skipped, not re-sent). The 7 pre-existing send.test.ts tests are untouched (pure append).
  3. Precondition-vs-SHOULD wording (minor) — reworded. specs/mail/sending.md §4 no longer calls provider Message-ID dedup a "precondition" — it's a recommendation (SHOULD); the engine's structural at-least-once guarantee (§3a) holds with or without it.

Full gates (typecheck, lint, full test suite) all green: 327/327 tests passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant